Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reorder evmc_message fields #128

Merged
merged 2 commits into from
Sep 3, 2018
Merged

Reorder evmc_message fields #128

merged 2 commits into from
Sep 3, 2018

Conversation

chfast
Copy link
Member

@chfast chfast commented Sep 3, 2018 •
edited
Loading

Resolves #123.
Resolves #122.

@chfast chfast force-pushed the message-refactor branch 2 times, most recently from f0c376a to cfe7bd9 Compare September 3, 2018 14:26
@chfast chfast changed the title Reorder evmc_message and evmc_result fields Reorder evmc_message fields Sep 3, 2018
@chfast chfast removed the in progress label Sep 3, 2018
@chfast chfast requested review from axic and gumb0 September 3, 2018 14:44
axic
axic reviewed Sep 3, 2018
View reviewed changes
include/evmc/evmc.h
@@ -70,58 +70,58 @@ enum evmc_flags
*/
struct evmc_message
{
/** The kind of the call. For zero-depth calls ::EVMC_CALL SHOULD be used. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this isn't changed in this PR, but I think this comment is invalid, it can be create even for depth 0.

@axic
Copy link
Member

axic commented Sep 3, 2018

So didn't #123 say that move code_hash from message to execute?

@chfast
Copy link
Member Author

chfast commented Sep 3, 2018

So didn't #123 say that move code_hash from message to execute?

Looks like it is not need at the moment, unless WAVM needs it. It was never taken from the database, was computed on the fly so VM is capable of doing the same.

@axic
Copy link
Member

axic commented Sep 3, 2018

Looks like it is not need at the moment, unless WAVM needs it. It was never taken from the database, was computed on the fly so VM is capable of doing the same.

With the extcodehash opcode in Constantinople will that stay like that?

@chfast
Copy link
Member Author

chfast commented Sep 3, 2018

The extcodehash is not useful here because you don't know the address of the code.

@axic
Copy link
Member

axic commented Sep 3, 2018

The extcodehash is not useful here because you don't know the address of the code.

Isn't #123 saying that execute() should have a code_hash parameter? The code is certainly known there.

@chfast
Copy link
Member Author

chfast commented Sep 3, 2018

Yes it says that, but now I prefer to skip it.

@axic
Copy link
Member

axic commented Sep 3, 2018

I don't need it myself so OK to remove it.

@chfast
Copy link
Member Author

chfast commented Sep 3, 2018

I prefer add features when there is use for them. And execute() together with message is a ton of params anyway.

@chfast chfast merged commit d617e19 into master Sep 3, 2018
@chfast chfast deleted the message-refactor branch September 3, 2018 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axic axic axic approved these changes

@gumb0 gumb0 Awaiting requested review from gumb0

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chfast @axic